Fix service_create status/connection string bug when waiting#53
Merged
nathanjcochran merged 2 commits intomainfrom Oct 10, 2025
Merged
Fix service_create status/connection string bug when waiting#53nathanjcochran merged 2 commits intomainfrom
service_create status/connection string bug when waiting#53nathanjcochran merged 2 commits intomainfrom
Conversation
nathanjcochran
commented
Oct 10, 2025
|
|
||
| // waitForServiceReady polls the service status until it's ready or timeout occurs | ||
| func waitForServiceReady(client *api.ClientWithResponses, projectID, serviceID string, waitTimeout time.Duration, output io.Writer) (*api.DeployStatus, error) { | ||
| func waitForServiceReady(client *api.ClientWithResponses, projectID, serviceID string, waitTimeout time.Duration, initialStatus *api.DeployStatus, output io.Writer) (*api.DeployStatus, error) { |
Member
Author
There was a problem hiding this comment.
This fixes a small bug I noticed when in the command version of waitForServiceReady, where the lastStatus returned from the command would be nil if it timed out before the first poll. Unlikely in practice, but technically possible if the --wait-timeout is less than 10 seconds (our polling interval).
The fix is just to pass in the service's initial status (from the create/fork request), and set the initial value of lastStatus to that.
nathanjcochran
commented
Oct 10, 2025
Comment on lines
-561
to
+563
| output.Service, err = s.waitForServiceReady(apiClient, cfg.ProjectID, serviceID, timeout, serviceStatus) | ||
| if err != nil { | ||
| if status, err := s.waitForServiceReady(apiClient, cfg.ProjectID, serviceID, timeout, service.Status); err != nil { | ||
| output.Message = fmt.Sprintf("Error: %s", err.Error()) | ||
| } else { | ||
| output.Service.Status = util.DerefStr(status) |
Member
Author
There was a problem hiding this comment.
This is the meat of the fix. Rather than overwriting output.Service with the latest polling result, we just overwrite output.Service.Status now (ensuring we don't overwrite the connection string/password set above).
murrayju
approved these changes
Oct 10, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes a small bug I didn't notice when I first implemented #50.
Specifically, when the
service_createMCP tool is called withwith_password: trueandwait: true, the password and connection string weren't being returned in the output, because after we finished polling to check if the service is ready, we replaced the entire service struct (which we had already modified with the connection string/password).This fixes it by making the MCP version of
waitForServiceReadyjust return the latest service status, rather than the entire service. This makes it work more like the command version ofwaitForServiceReady(we should really consolidate them, but I didn't want to deal with the logging/output differences right now - can tackle it if/when we overhaul logging in general).Also fixed a very small bug in the command version of
waitForServiceReady. See comment below.